Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Try to Fix Flaky Test #27939

Merged
merged 1 commit into from
Aug 27, 2023
Merged

Try to Fix Flaky Test #27939

merged 1 commit into from
Aug 27, 2023

Conversation

smallinsky
Copy link
Contributor

@smallinsky smallinsky commented Jun 16, 2023 •
edited
Loading

What:

Try to fix test flakiness issues: #13623 and #13944 caused by racate between t.TmpDir cleanup and session recording handler:

ERROR: access denied to invalidUser connecting to localhost:24915

    testing.go:1206: TempDir RemoveAll cleanup: unlinkat /tmp/TestTSHConfigConnectWithOpenSSHClientnode_recording_mode_with_TLS_routing_disabled2058613825/001/log/upload/streaming/default: directory not empty
--- FAIL: TestTSHConfigConnectWithOpenSSHClient/node_recording_mode_with_TLS_routing_disabled (3.93s)
  • Force the node-sync and add missing node.Wait() call
  • Move dynamicServiceAddr to common package and reuse. (TestTerminalRequireSessionMfa)
  • Fix flaky way of acquiring open local port by using dynamicServiceAddr

@smallinsky smallinsky requested a review from Tener June 16, 2023 14:55
@Tener
Copy link
Contributor

Tener commented Jun 16, 2023

I'll need to dig it out, but from memory, the common fix was to remove session recording from happening in tests? @zmb3

@zmb3
Copy link
Collaborator

zmb3 commented Jun 30, 2023

I'll need to dig it out, but from memory, the common fix was to remove session recording from happening in tests? @zmb3

Yes, addressed that in #28518

@smallinsky smallinsky marked this pull request as ready for review July 3, 2023 10:43
@github-actions github-actions bot requested review from camscale and ravicious July 3, 2023 10:44
@github-actions github-actions bot added size/sm tctl tctl - Teleport admin tool tsh tsh - Teleport's command line tool for logging into nodes running Teleport. labels Jul 3, 2023
@smallinsky
Copy link
Contributor Author

Friendly ping @Tener

@Tener
Copy link
Contributor

Tener commented Jul 21, 2023

Is this PR still needed? I thought #28518 would provide a fix already. If not, do let me know, and I'll review it.

@smallinsky
Copy link
Contributor Author

Is this PR still needed? I thought #28518 would provide a fix already. If not, do let me know, and I'll review it.

Yea this is still needed. This pr aims to fix the TestTSHConfigConnectWithOpenSSHClientnode_recording_mode_with_TLS_routing directory not empty flaky test issue where #28518 addressed only TestTerminalRequireSessionMFA

@public-teleport-github-review-bot public-teleport-github-review-bot bot removed the request for review from camscale July 21, 2023 14:55
@zmb3
Copy link
Collaborator

zmb3 commented Aug 6, 2023

@smallinsky do we want to merge this still?

@smallinsky
Copy link
Contributor Author

@zmb3

do we want to merge this still?
Yes thanks for reminding me about this. Can I ask for UT "Flaky Tests Detector" skip due to test --count=100 timeout ?

@zmb3
Copy link
Collaborator

zmb3 commented Aug 8, 2023

/excludeflake TestTokens TestConnect TestCreateSAMLIdPServiceProvider TestCreateClusterAuthPreference_WithSupportForSecondFactorWithoutQuotes TestCreateDatabaseInInsecureMode TestCreateLock TestIntegrationResource TestAppResource TestDatabaseServerResource TestDatabaseResource TestDatabaseServiceResource

@zmb3
Copy link
Collaborator

zmb3 commented Aug 23, 2023

/excludeflake *

@zmb3
Copy link
Collaborator

zmb3 commented Aug 23, 2023

@smallinsky flake detector bypass should be good now. Let's get this rebased and merged.

@smallinsky smallinsky force-pushed the smallinsky/try_fix_flacky_test_tmp_dir branch from 12ad0f0 to d44d011 Compare August 27, 2023 18:58
@smallinsky smallinsky added this pull request to the merge queue Aug 27, 2023
@smallinsky smallinsky removed this pull request from the merge queue due to a manual request Aug 27, 2023
@smallinsky smallinsky force-pushed the smallinsky/try_fix_flacky_test_tmp_dir branch from d44d011 to c2be7d4 Compare August 27, 2023 19:40
@smallinsky smallinsky enabled auto-merge August 27, 2023 19:40
@smallinsky smallinsky added this pull request to the merge queue Aug 27, 2023
Merged via the queue into master with commit 21e9b85 Aug 27, 2023
@smallinsky smallinsky deleted the smallinsky/try_fix_flacky_test_tmp_dir branch August 27, 2023 20:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Tener Tener Tener approved these changes

@ravicious ravicious ravicious approved these changes

Assignees
No one assigned
Labels
flaky tests size/sm tctl tctl - Teleport admin tool tsh tsh - Teleport's command line tool for logging into nodes running Teleport.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@smallinsky @Tener @zmb3 @ravicious